Valueless keys #12

khuey · 2017-12-21T04:37:41Z

In environment sections you can specify valueless keys like

environment:
  - FOO
  - BAR

And docker-compose will pick up the relevant values on the machine compose runs on.

The other form,

environment:
  FOO:
  BAR:

would require serde_yaml changes, I believe.

…he host at runtime) values. environment: - SECRET

dtolnay · 2017-12-21T05:38:46Z

Serde_yaml sees the second form as having null values. This matches other YAML libraries I tried.

extern crate serde_yaml;
use serde_yaml::Value;

const Y: &str = "
---
environment:
  FOO:
  BAR:
";

fn main() {
    println!("{:#?}", serde_yaml::from_str::<Value>(Y).unwrap());
}

emk

Thank you for this patch! I think I recall seeing this in the docker-compose.yml standard, and I would love to have a compliant implementation of this part of the standard.

compose_yml currently supports the docker-compose.yml specifications for versions "2" and "2.1" (and we could consider adding support for later "2.x" versions pretty easily, though "3.x" would probably be harder). In particular, we want to make sure that we don't support any non-compliant extensions and that we conform strictly to the standard.

So in order to help us remain compliant, could you please point me to where this behavior is specified in the docker-compose.yml specs? This will make it easier for code reviewers. (And we need to be careful, given that schema validation is currently broken because of s-panferov/valico#27).

Also, this PR has no test cases. Could you please look at the various assert_roundtrip! test cases in the code base, and make sure that there are test cases for this feature, showing that it handles (1) all the old behaviors, and (2) the new behavior? Thank you!

To do items:

A link to the relevant part of the standard for version "2" or "2.1".
Test cases.
Other issues mentioned below.

Thank you for this PR!

emk · 2017-12-21T12:06:43Z

src/v2/service.rs

-            new_env.append(&mut env_file.to_environment()?);
+            new_env.extend(env_file.to_environment()?
+                           .into_iter()
+                           .map(|(k, v)| (k, Some(v))));


If environments now have type BTreeMap<String, Option<RawOr<String>>>, then we should consider updating to_environment to also return that type, instead of patching it up here. Or is something more complicated going on that we need to think about first?

Turns out env_files also accept valueless keys, so we should just handle them there and produce the same BTreeMap type.

emk · 2017-12-21T12:09:50Z

src/v2/helpers.rs

+        }
+    }
+
+    deserializer.deserialize_map(MapOrKeyOptionalValueListVisitor)


Just last night, I discovered in serde-rs/serde#1125 (comment) that there has been a breaking change to the serde crate, and it is no longer safe to call deserialize_map as a hint when when override both visit_map and visit_seq. This needs to be deserialize_any (and I need to fix this everywhere else in compose_yml as well). Sorry for this last minute surprise. :-(

I fixed this, and in the other obvious places too.

emk · 2017-12-21T12:11:19Z

src/v2/helpers.rs

@@ -95,7 +95,7 @@ impl<'de> Deserialize<'de> for ConvertToString {
 ///
 /// ```text
 /// struct Example {
-///     #[serde(deserialize_with = "deserialize_hash_or_key_value_list")]
+///     #[serde(deserialize_with = "deserialize_map_or_key_value_list")]


Good catch!

emk · 2017-12-21T12:13:33Z

src/v2/service.rs

@@ -72,8 +72,8 @@ pub struct Service {

    /// Environment variables and values to supply to the container.
    #[serde(default, skip_serializing_if = "BTreeMap::is_empty",
-            deserialize_with = "deserialize_map_or_key_value_list")]
-    pub environment: BTreeMap<String, RawOr<String>>,
+            deserialize_with = "deserialize_map_or_key_optional_value_list")]


Have we looked at all the other callers of deserialize_map_or_key_value_list and verified that none of them require similar conversions? I can check this if you want, but it will probably have to wait until sometime next week at least.

I tested all of them with docker_compose and the only one that doesn't accept valueless keys is driver_opts. All the others do.

This raises some mildly interesting questions in cage, such as what to do if io.fdy.cage.srcdir is set but null.

khuey · 2017-12-21T13:37:42Z

I don't know if there's a spec for docker-compose.yml beyond the reference, but it's documented at https://docs.docker.com/compose/compose-file/compose-file-v2/#environment with no minor version qualifications.

The reason I didn't add an assert_roundtrip! test is that this already fails even for keys with values because compose_yml canonicalizes the array form to the object form. i.e.

environment:
  - FOO=bar

fails an assert_roundtrip! with

  left: `Object({"environment": Object({"FOO": String("bar")})})`,
 right: `Object({"environment": Array([String("FOO=bar")])})`'

docker-compose doesn't seem to document this behavior but it does accept these.

Through testing, only the `driver_opts` subkey of `volumes` actually rejects missing values. Everything else that uses `deserialize_map_or_key_value_list` does accept them.

khuey · 2018-01-02T03:39:28Z

What else needs to happen here?

emk · 2018-01-02T11:24:59Z

src/v2/env_file.rs

@@ -79,6 +82,7 @@ fn parses_docker_compatible_env_files() {
 # These are environment variables:
 FOO=foo
 BAR=2
+BAZ


As far as I can tell from the documentation, this is a non-standard extension to the .env format:

Compose expects each line in an env file to be in VAR=VAL format. Lines beginning with # (i.e. comments) are ignored, as are blank lines.

So unless we can find some evidence that Docker supports this extension, we should remove this code.

emk

Thank you for the update!

We need to either justify or remove the support for empty keys in .env.

emk · 2018-01-02T11:26:27Z

src/v2/build.rs

               "value");
+    assert_eq!(*build.args.get("other_key").expect("should have other_key"), None);


OK, good, this is the main test that we need.

khuey · 2018-01-02T15:53:34Z

I don't see it documented anywhere but it does happen.

khuey@workspace:~/scratch/docker-test$ docker -v
Docker version 1.13.1, build 092cba3
khuey@workspace:~/scratch/docker-test$ docker-compose -v
docker-compose version 1.18.0, build 8dd22a9
khuey@workspace:~/scratch/docker-test$ cat docker-compose.yml
test:
  image: jwilder/whoami
  env_file: test.env
khuey@workspace:~/scratch/docker-test$ cat test.env
TEST_VALUE
khuey@workspace:~/scratch/docker-test$ export TEST_VALUE=Hi
khuey@workspace:~/scratch/docker-test$ docker-compose up -d
Recreating dockertest_test_1 ... done
khuey@workspace:~/scratch/docker-test$ docker ps
CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES
56dfcff90ec5        jwilder/whoami      "/app/http"         3 seconds ago       Up 2 seconds        8000/tcp            dockertest_test_1
khuey@workspace:~/scratch/docker-test$ docker exec -it 56df sh
/app # echo $TEST_VALUE
Hi

khuey · 2018-01-02T15:55:33Z

(docker-compose behaves this way with an explicit version: "2" in the compose file as well)

emk · 2018-01-02T16:32:06Z

@khuey OK, if docker-compose supports it, we can certainly implement it.

But please keep in mind my comments on faradayio/cage#87 (comment). We don't have any kind of coherent, principled model for how cage should handle env interpolation, so I'd be careful about relying on these features too heavily if you're using them with cage—at least until we have solid conceptual model about how they should work.

Anyway, this looks about ready to merge. If you don't hear from me by Wednesday, please ping me.

khuey added 2 commits December 19, 2017 19:02

Fix doc comment.

f43de64

Add support for key/value maps with unspecified (i.e. gathered from t…

b3cae95

…he host at runtime) values. environment: - SECRET

khuey mentioned this pull request Dec 21, 2017

Handle compose_yml changes for valueless keys faradayio/cage#84

Open

emk requested changes Dec 21, 2017

View reviewed changes

khuey added 3 commits December 21, 2017 09:30

Handle environment variables with no value in env files as well.

fe62450

docker-compose doesn't seem to document this behavior but it does accept these.

Replace deserialize_map with deserialize_any.

269d68d

Allow valueless keys everywhere docker-compose does.

ddc591d

Through testing, only the `driver_opts` subkey of `volumes` actually rejects missing values. Everything else that uses `deserialize_map_or_key_value_list` does accept them.

emk reviewed Jan 2, 2018

View reviewed changes

emk requested changes Jan 2, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Valueless keys #12

Valueless keys #12

khuey commented Dec 21, 2017

dtolnay commented Dec 21, 2017

emk left a comment

emk Dec 21, 2017 •

edited

Loading

khuey Dec 21, 2017

emk Dec 21, 2017

khuey Dec 21, 2017

emk Dec 21, 2017

emk Dec 21, 2017

khuey Dec 21, 2017

khuey commented Dec 21, 2017 •

edited

Loading

khuey commented Jan 2, 2018

emk Jan 2, 2018

emk left a comment

emk Jan 2, 2018

khuey commented Jan 2, 2018

khuey commented Jan 2, 2018

emk commented Jan 2, 2018

		"value");
		assert_eq!(*build.args.get("other_key").expect("should have other_key"), None);

Valueless keys #12

Are you sure you want to change the base?

Valueless keys #12

Conversation

khuey commented Dec 21, 2017

dtolnay commented Dec 21, 2017

emk left a comment

Choose a reason for hiding this comment

emk Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khuey commented Dec 21, 2017 • edited Loading

khuey commented Jan 2, 2018

Choose a reason for hiding this comment

emk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khuey commented Jan 2, 2018

khuey commented Jan 2, 2018

emk commented Jan 2, 2018

emk Dec 21, 2017 •

edited

Loading

khuey commented Dec 21, 2017 •

edited

Loading